-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lint: Add support for CSS stylesheets in linting workflow. #66
Conversation
… failed although the steps are continue on error.
…un lint:css-check".
new-log-viewer/package.json
Outdated
"lint:fix": "npm-run-all --sequential --continue-on-error lint:css-fix lint:js-fix", | ||
|
||
"lint:css-check": "stylelint src/**/*.css", | ||
"lint:css-ci": "npm run lint:css-check -- --formatter github", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.github/workflows/lint.yaml
Outdated
@@ -29,4 +29,6 @@ jobs: | |||
with: | |||
node-version: 22 | |||
- run: "npm --prefix new-log-viewer/ clean-install" | |||
- run: "npm --prefix new-log-viewer/ run lint:css-ci" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just update this now to call lint:check:js and lint:check:css so lint:check dosen't run lint:css twice
.github/workflows/lint.yaml
Outdated
@@ -29,4 +29,6 @@ jobs: | |||
with: | |||
node-version: 22 | |||
- run: "npm --prefix new-log-viewer/ clean-install" | |||
- run: "npm --prefix new-log-viewer/ run lint:css-ci" | |||
- run: "npm --prefix new-log-viewer/ run lint:check" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I think this was supposed to be lint:js-check
.
- run: "npm --prefix new-log-viewer/ run lint:check" | |
- run: "npm --prefix new-log-viewer/ run lint:js-check" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you rid of this later anyways. so i'm ignoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it turn out that no changes are needed in the workflow file after the scripts are simplified
"lint:check": "npm-run-all --sequential --continue-on-error lint:check:*", | ||
"lint:check:css": "stylelint src/**/*.css --formatter github", | ||
"lint:check:js": "eslint src webpack.*.js --max-warnings 0", | ||
"lint:fix": "npm-run-all --parallel --continue-on-error \"lint:check:* -- --fix\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the parallel might be slightly more convenient, but maybe could cause issues if the linters touch the same files? If they don't, im okay with it. If they do, maybe just use sequential to be safer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the linters touch the same files
Supposedly no at the moment. ESLint would only look into js, jsx, ts, and tsx files and we're asking Stylelint to look only into css files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay ltgm
.github/workflows/lint.yaml
Outdated
@@ -29,4 +29,6 @@ jobs: | |||
with: | |||
node-version: 22 | |||
- run: "npm --prefix new-log-viewer/ clean-install" | |||
- run: "npm --prefix new-log-viewer/ run lint:css-ci" | |||
- run: "npm --prefix new-log-viewer/ run lint:check" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you rid of this later anyways. so i'm ignoring
new-log-viewer/package.json
Outdated
"lint:fix": "npm run lint:check -- --fix", | ||
"start": "webpack serve --open --config webpack.dev.js" | ||
"lint:check": "npm-run-all --sequential --continue-on-error lint:check:*", | ||
"lint:check:css": "stylelint src/**/*.css --formatter github", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to do this or just use the string formatter... I dont mind looking at logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I think we can add the problem matcher for the string
formatter in another PR soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I make the changes let's take a look at the approaches we can take:
- Use a custom formatter in Stylelint and a custom action in the GitHub workflow.
PROS: We can view the errors in a nicely formatted way and we can have the errors annotated inline by GitHub.
CONS: setting up custom stuff takes time; we might not want to use custom stuff given they might not always be compatible with the tooling upgrades / GitHub workflow, which means we might not want to maintain it as a long term solution. - Use the default
string
formatter.
PROS: It's the default formatter which means it should have the best support from the tooling contributors.
CONS: We lose the ability to have the errors annotated inline (until we move the files into the project root). It outputs relative paths, but GitHub workflow problem matchers only resolves paths from the project root. Supposedly we can check in the problem matcher, which currently does nothing, in the same PR to save another PR, but it's hard to verify if things would actually work. If we spot any issues after we move the log-viewer files, we might still need to fix in the PR we move the files or another PR. - Use the
github
formatter.
PROS: We don't need any custom plugin to get the GitHub annotation working.
CONS: We will see a deprecated warning in the workflow results. We're using a deprecated but not yet removed formatter. If we don't upgrade the Stylelint version the formatter should still be there, and it's less likely we would upgrade it before we move the new-log-viewer files and migrate to the problem matcher approach.
I still think Approach 3 gives us the most benefits, but all CONs are minor since we might always need another PR to improve the lint experience after we move the new-log-viewer files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay i think it dosen't matter so much as we are going to move soon anyways. I am okay with 3.
Do you like this by any chance? This way we still preserve normal formatting when running locally (i.e. run lint:check locally and lint:ci for workflow)
"lint:ci": "npm-run-all --sequential --continue-on-error lint:check:js \"lint:check:css -- --formatter github\""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you would need to modify workflow to use lint:ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im good! Do you like for title "lint: Add support for CSS stylesheets in linting workflow."
References
new-log-viewer series: #45 #46 #48 #51 #52 #53 #54 #55 #56 #59 #60 #61 #62 #63
Description
Validation performed
Created test branch for testing PR submitting to main: junhaoliao#6